Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
|
I am not sure which one will go first. If these changes are independent, it would make more sense to submit them as independent PRs. |
ffa8a51 to
695f29e
Compare
jkotas
left a comment
There was a problem hiding this comment.
One of the Microsoft maintainers will have to run internal VS debugger tests on this before it gets merged.
|
Failures are known, the new one is unrelated deadlettered leg. Results still match before/after #126809 (comment) |
|
Let me run the private diagnostic tests with this change. |
|
There is about 15 failed tests with this change (compared to the commit right before your change). Moreover, I've found that we also have some additional failures in these tests that one of your previous changes - #126222 has introduced. Since I have run those tests at one point during your PR, it seems that some changes made after that has introduced the problem. |
|
So, regarding the issues introduced by the old PR, the DacDbiInterfaceImpl::UnwindStackWalkFrame needs to be updated to skip frames of "System.Environment.CallEntryPoint". I've tried a quick hack (just comparing method name string) and found that fixes two of the three EH related failures. The remaining issue from the old PR is strange. The debugger can see a stack where the System.Environment.CallEntryPoint is at the top of the stack, with Main at the bottom. That doesn't make sense, I am looking at the test to see if I can isolate a standalone repro. |
Moved those changes to #126927 and reverted here. |
| // | ||
| // Do Step 1e - Gather info from runtime about args (may trigger a GC). | ||
| // | ||
| // GC-protect all arg addresses as interior pointers before any GC-triggering |
There was a problem hiding this comment.
We are calling AllocateObject above that can trigger GC. How are the pointers inside GetArgData protected during that AllocateObject?
There was a problem hiding this comment.
The pointers inside GetArgData are now GC-protected as interior pointers before AllocateObject (and all other GC-triggering calls). I've moved the pArgAddrs setup + GCPROTECT_BEGININTERIOR_ARRAY to the very top of DoNormalFuncEval, before ResolveFuncEvalGenericArgInfo and AllocateObject.
|
@am11 even after your last change it doesn't work. I can see that the DoNormalFunceval took the expected path, the pDE->m_resultType was System.String, the ucoGc.resultObj was of type System.String. So I've spent more time debugging it and it turns out the problem is actually at the funceval arguments processing. The thing is that the debugger calls ToString on the struct via funceval. But we end up boxing the "this" argument. The original code didn't do that - dumping the type of *pObjectRefArray after the call to BoxFuncEvalThisParameter is the struct in the old state case, but the type ucoGc.thisArg in your case is System.Object. |
|
It seems the original code would only box "this" argument if |
|
@janvorli, based on your description, I have pushed a change. When |
|
@am11 the issue with structs, delegates and generic structs is now gone. However, it breaks the funceval test of a method returning bool - it now returns wrong value (true instead of false). And there is another similar case when a funceval of float returning method returns completely wrong float. I think you probably cannot handle regular args and this the same way - the original code had a separate methods handling of args and this. |
|
@janvorli, that value type regression was caused by previous commit. It was checking if objectref is value type rather than its corelement type. |
|
@am11 now all the failures I've seen are gone. |
|
@dotnet/dotnet-diag-contrib Any concerns with this change? It is switching funceval to use reflection invoke. It makes the code a lot simpler and fully portable that is going to help with the wasm port. |
noahfalk
left a comment
There was a problem hiding this comment.
This seems like a great simplification. Looking over it it appears to be doing the right things. There are lots of subtleties I'm unlikely to catch just by reading the code so I'm also relying heavily on the testing you did @janvorli (thanks!)
@tommcdon - is there any additional testing we could do on this, perhaps VS, that would help flush out lingering issues?
|
Thanks, will keep an eye out post-merge for any fallouts after daily SDK is released. (I think folks will be able to test conveniently after the signed binaries are out) |
|
@janvorli, @tommcdon, a note: this branch should be built from clean state (i.e. after deleting artifacts dir). I noticed that cmake or ninja doesn't reinstall some DAC related stuff if we built main before rebuilding this branch and we run into mismatch error when using debugger in VS with |



Built on top of #126542.Last part of #123864 before the final cleanups.